Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore client connection closing - simpler challenge response error handling #18

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

therealpaulgg
Copy link
Owner

There were significant oversights in the pull request #17.

While this code provided a great way of determining if the client indeed killed their session, there was a significant issue that arose, where this goroutine would read the first byte from the connection, causing any other readers which relied on this byte to effectively break themselves.

This code has been greatly simplified to, on the challenge responder side, simply check to see if we are receiving data from the channel (the channel closes, returning nil bytes). This was previously assuming any data obtained from the channel was in fact a byte array, which was false.

This code also resolves another bug, in which there was a goroutine reading from a channel from which the main thread was reading. While this code went unnoticed for a while, it was the cause of earlier panic issues (which were later obscured by concurrency bug fixes). The main channel always wins the race condition; but if it were to lose, this would result in a deadlock. The code has been simplified to have a channel created on the main thread, which the goroutine responds to either the timer expires or there is a message received from the Challenge Accepted channel (which other goroutines write to).

@therealpaulgg therealpaulgg merged commit 2d7c1aa into main Mar 22, 2024
1 check passed
@therealpaulgg therealpaulgg deleted the fixing-critical-bugs branch March 22, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant